-
Notifications
You must be signed in to change notification settings - Fork 151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle a version with a dash in the prerelease tag #60
Handle a version with a dash in the prerelease tag #60
Conversation
When parsing a constraint, before converting dashes to a range, check if it is a valid version first. That way a version with multiple dashes, like v1.0.0-12-abc123 isn't turned into a range of 1.0.0 to 12-abc123.
af60833
to
a93e51b
Compare
My initial attempt checked if the version was valid using only the regex, but that doesn't work well when legitimate ranges look exactly like versions, such as |
@@ -13,6 +13,9 @@ func rewriteRange(i string) string { | |||
} | |||
o := i | |||
for _, v := range m { | |||
if strings.HasPrefix(v[0], "v") && versionRegex.MatchString(v[0]) { | |||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the method of detecting a leading v
. The leading v
is not part of the SemVer spec. Applications other than dep import this package and don't require the v
the way the go community wants to.
Any ideas on another method? The 1.x version looks for a space on both sides of the - for a range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for letting this drop off my radar entirely!
Would you be up for requiring that ranges must have a space around the dash? Right now the v2 branch has tests that support 2-3
which made it nearly impossible for me to do what I needed without the leading v trick. If we went back to requiring spaces for a range, then it wouldn't be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carolynvs sorry this dropped of my radar. If there was a space surrounding the dash it would work for me. The currently implementation on the v1 branch requires one or more space characters on each side of the dash.
This would be so much easier if there was a spec for ranges.
Closing in favor of #67 |
@carolynvs, dep's Gopkg file is broken since your branch is gone. https://github.com/golang/dep/blob/master/Gopkg.toml#L1 . Can you please look into this? |
@tamalsaha I've repushed the branch, sorry about that. Can you help me understand what exactly broke immediately once the branch was gone? e.g. building dep, or something else? |
Thanks @carolynvs ! We use the gps library in https://github.com/kubepack/pack . So, I tried to revendor and it broke. |
Doh! Sorry about that. GitHub gave me a big button to press when I closed the PR and ... I have no self control. 😂 I promise to keep that branch around forever and ever now. |
Working on getting dep in sync with the 2.x branch here: golang/dep#1792 |
When parsing a constraint, before converting dashes to a range, check if it is a valid version first. That way a version with multiple dashes, like
v1.0.0-12-abc123
isn't turned into a range of1.0.0
to12-abc123
.Fixes #59.